Fix URL change detection broken by database search and replace by shervElmi · Pull Request #13135 · google/site-kit-wp

shervElmi · 2026-07-17T18:28:14Z

Summary

Addresses issue:

Search and replace breaks URL change detection #12962

Relevant technical choices

The sanitize callback encodes as well as set(), so a value written straight to the option, as the options screen writes it, still stores encoded.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 7.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.
Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2026-07-17T18:29:11Z

🤖 This comment is automatically updated by CI workflows. Each section is managed independently.

🎭 Playwright reports for e35bb46:

📦 Build files for e35bb46:

Download development build
Download production build

Store the setting base64-encoded, so a database search and replace leaves it alone and URL change detection keeps working. The getter decodes on read, and a value an earlier version stored in plain text passes through unchanged.

Re-save the connected proxy URL on admin_init, so a value an earlier version stored in plain text becomes encoded. The migration runs once and then stamps the DB version option.

Add the migration to the plugin's registration list, so it runs alongside the earlier migrations.

Rephrase comments to enhance understanding of how legacy values are stored and processed. This improves code readability and maintainability.

tofumatt

I know the original IB mentioned supporting unencoded URLs… but why?

I've asked @aaemnnosttv about this since he wrote the issue/ACs. But if that isn't needed I think we can simplify this implementation 🙂

tofumatt · 2026-07-23T11:22:58Z

+	 * @since n.e.x.t Compares against the decoded setting value.
 	 *
-	 * @param string $url URL to match against the current one in the settings.
+	 * @param string $site_url URL to match against the current one in the settings.
 	 * @return bool TRUE if URL matches the current one, otherwise FALSE.
 	 */
-	public function matches_url( $url ) {
-		$sanitize   = $this->get_sanitize_callback();
-		$normalized = $sanitize( $url );
-		return $normalized === $this->get();
+	public function matches_url( $site_url ) {
+		return trailingslashit( $site_url ) === $this->get();
+	}


Why change the argument name here? We're still just comparing a URL; the function doesn't require it be the site URL so this is just slightly confusing to me.

tofumatt · 2026-07-23T11:26:20Z

+	 *
+	 * @since n.e.x.t
+	 *
+	 * @param string $value Connected proxy URL, either plain text or encoded.


I know the IB mentions supporting plaintext, but do we really need to do that in the set method? Seems like we could just have a migration for this data instead, and then simplify our actual code here to not have to check for https?:\/\/ at the start of the URL… 🤔

tofumatt · 2026-07-23T11:27:25Z

+	 * The encoded value holds no readable URL, so a database search and
+	 * replace leaves it unchanged. An already encoded value decodes first,
+	 * so a re-save never encodes it twice.


Suggested change

* The encoded value holds no readable URL, so a database search and

* replace leaves it unchanged. An already encoded value decodes first,

* so a re-save never encodes it twice.

* We encode the URL to prevent database-wide search-and-replace tasks

* from changing the URL used to connect to the Site Kit Proxy service.

tofumatt · 2026-07-23T11:27:57Z

+	 *
+	 * @since n.e.x.t
+	 *
+	 * @param string $value Connected proxy URL, either plain text or encoded.


Similarly… why would we ever pass an already-encoded value to this function? 🤔

tofumatt · 2026-07-23T11:29:25Z

If we already have this migration, why do we need to account for both encoded and unencoded values in the Connected_Proxy_URL file? 🤔

Seems we can assume they're always encoded, no?

tofumatt · 2026-07-23T11:38:06Z

+	public function test_get__returns_a_legacy_plain_text_url() {
+		$connected_proxy_url = new Connected_Proxy_URL( $this->options );
+
+		// Store the option in plain text, the way earlier plugin versions saved it.
+		$this->update_option( 'https://example.com/' );
+
+		$connected_proxy_url->register();
+
+		$this->assertEquals( 'https://example.com/', $connected_proxy_url->get(), 'Getter should return a legacy plain text value unchanged.' );
+	}
+


Suggested change

public function test_get__returns_a_legacy_plain_text_url() {

$connected_proxy_url = new Connected_Proxy_URL( $this->options );

// Store the option in plain text, the way earlier plugin versions saved it.

$this->update_option( 'https://example.com/' );

$connected_proxy_url->register();

$this->assertEquals( 'https://example.com/', $connected_proxy_url->get(), 'Getter should return a legacy plain text value unchanged.' );

}

As mentioned, I think we can remove this.

tofumatt · 2026-07-23T11:39:17Z

+	public function test_get__returns_a_value_that_fails_to_decode_unchanged() {
+		$connected_proxy_url = new Connected_Proxy_URL( $this->options );
+
+		$this->update_option( 'not*a*valid*value' );
+
+		$connected_proxy_url->register();
+
+		$this->assertEquals( 'not*a*valid*value', $connected_proxy_url->get(), 'Getter should leave a value that fails to decode unchanged.' );
+	}


When this happens in the plugin, what error message is surfaced? 🤔

This assertion now expects that FALSE, renamed to test_get__returns_false_for_a_value_that_fails_to_decode.

tofumatt · 2026-07-23T11:39:41Z

+		$connected_proxy_url = new Connected_Proxy_URL( $this->options );
+		$connected_proxy_url->register();
+
+		// Save the option directly, as an update on the options screen would.


Which options screen? 🤔

The built-in WordPress "All Settings" screen, wp-admin/options.php. I've removed that test and the sanitize callback, so that path doesn't apply anymore.

tofumatt · 2026-07-23T11:40:24Z

+			$this->options->get( Connected_Proxy_URL::OPTION ),
+			'Sanitize callback should encode a value that update_option saves.'
+		);
+		$this->assertEquals( 'https://example.com/', $connected_proxy_url->get(), 'Getter should return the plain URL after a direct save.' );


When is this a valid scenario? Users shouldn't be arbitrarily editing our settings…

Yeah, it's not really a valid scenario! 👍🏻 I've removed the test.

tofumatt · 2026-07-23T11:42:09Z

+		$this->assertFalse( $connected_proxy_url->matches_url( 'https://new-site.example.com' ), "Stored URL shouldn't match the new URL, so Site Kit detects the change." );
+	}
+
+	public function test_matches_url__flags_a_url_change_after_a_database_search_and_replace() {


This isn't so much simulating a search-and-replace as a general home_url change, which a search-and-replace would include but is a subset of home URL changes.

Let's update the test name and comments/text to reflect this. It's fine to be more general in this naming. 🙂

tofumatt

Let's assume the value is encoded going forward, see: https://fueled.slack.com/archives/C0788NZMLF2/p1784807108217289

We don't need to account for an unencoded value after the migration, so this can be simplified. I've updated the issue's ACs accordingly.

…onnected-proxy-url

shervElmi added 3 commits July 22, 2026 16:54

Encode the connected proxy URL setting.

3b6235f

Store the setting base64-encoded, so a database search and replace leaves it alone and URL change detection keeps working. The getter decodes on read, and a value an earlier version stored in plain text passes through unchanged.

Add the connected proxy URL migration.

2e8c6be

Re-save the connected proxy URL on admin_init, so a value an earlier version stored in plain text becomes encoded. The migration runs once and then stamps the DB version option.

Register the connected proxy URL migration.

984a68f

Add the migration to the plugin's registration list, so it runs alongside the earlier migrations.

shervElmi force-pushed the bug/12962-encode-connected-proxy-url branch from ef75334 to 984a68f Compare July 22, 2026 13:54

shervElmi marked this pull request as ready for review July 22, 2026 13:55

shervElmi added 2 commits July 22, 2026 18:31

Update comments for clarity in Connected_Proxy_URL and migrations.

d57a71a

Rephrase comments to enhance understanding of how legacy values are stored and processed. This improves code readability and maintainability.

Encode the connected proxy URL without a read filter.

7e1de03

tofumatt requested changes Jul 23, 2026

View reviewed changes

shervElmi added 3 commits July 24, 2026 00:34

Address CR feedback.

67a2678

Merge remote-tracking branch 'origin/develop' into bug/12962-encode-c…

d184b3b

…onnected-proxy-url

Fix PHP lint full-stop errors.

e35bb46

Uh oh!

Conversation

shervElmi commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shervElmi commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading